Skip to content

Add CI job to build C sources with various compiler versions and fix array-bounds error with GCC 9#4069

Merged
Earlopain merged 4 commits into
ruby:mainfrom
eregon:gcc-ci
Apr 29, 2026
Merged

Add CI job to build C sources with various compiler versions and fix array-bounds error with GCC 9#4069
Earlopain merged 4 commits into
ruby:mainfrom
eregon:gcc-ci

Conversation

@eregon
Copy link
Copy Markdown
Member

@eregon eregon commented Apr 6, 2026 •
edited
Loading

eregon added 2 commits April 6, 2026 14:37
@eregon
Add CI job to build C sources with various compiler versions
5187f69 
* See ruby#4048
@eregon
Fix array-bounds error with GCC 9
2908c1c 
* In file included from /usr/include/string.h:535,
                 from include/prism/internal/arena.h:12,
                 from src/prism.c:6:
  In function 'memset',
    inlined from 'lex_mode_push_regexp' at src/prism.c:290:5:
  .../string_fortified.h:59:10: error: '__builtin_memset' offset [26, 34] from the object at 'lex_mode' is out of the bounds of referenced subobject 'breakpoints' with type 'uint8_t[7]' {aka 'unsigned char[7]'} at offset 18 [-Werror=array-bounds]
@eregon eregon requested a review from kddnewton April 6, 2026 12:55
kddnewton
kddnewton previously requested changes Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the deal with the changes to parser.h here?

@eregon
Copy link
Copy Markdown
Member Author

eregon commented Apr 6, 2026

@kddnewton Could you check the commit message? It explains it.

@eregon
Copy link
Copy Markdown
Member Author

eregon commented Apr 6, 2026

The new job took 10min15s, which is faster than memcheck.
So maybe it's fast enough for now?

Not sure where the time is spent but I wondered about image size:

$ docker images
ghcr.io/ruby/ruby-ci-image                                gcc-15               771cea939adf  6 days ago      3.18 GB
docker.io/library/gcc                                     15                   9a52dbcd9b63  2 weeks ago     1.57 GB

Maybe we could use gcc:15 but then we'd also need ruby inside, or change Makefile to no longer depend on ruby for SOEXT (or just fallback to .so if no ruby).

@eregon eregon requested a review from kddnewton April 6, 2026 15:32
@eregon
Copy link
Copy Markdown
Member Author

eregon commented Apr 6, 2026 •
edited
Loading

@kddnewton Could you check the commit message? It explains it.

To explain a bit more, the code does memset(breakpoints, 0, PM_STRPBRK_CACHE_SIZE); but that's at least theoretically invalid because PM_STRPBRK_CACHE_SIZE=16 and breakpoints = lex_mode.as.list.breakpoints and that's defined as uint8_t breakpoints[11]; or uint8_t breakpoints[7];.
It's an out of bound access, but maybe it doesn't cause any issue in practice due to alignment and/or the union, nevertheless it seems far safer to use uint8_t breakpoints[PM_STRPBRK_CACHE_SIZE]; (and fixes the GCC 9 error).

@eregon
Copy link
Copy Markdown
Member Author

eregon commented Apr 6, 2026

I switched to gcc images and it's quite a bit faster :) 6m49s

@eregon eregon requested a review from Earlopain April 28, 2026 19:16
@eregon
Copy link
Copy Markdown
Member Author

eregon commented Apr 28, 2026

@Earlopain Could you help reviewing this?

Earlopain
Earlopain approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Earlopain Earlopain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks fine to me. Originally this was done to fix some ASAN errors 1cabef7

@Earlopain Earlopain merged commit 58b7cf5 into ruby:main Apr 29, 2026
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Earlopain Earlopain Earlopain approved these changes

@kddnewton kddnewton Awaiting requested review from kddnewton

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eregon @kddnewton @Earlopain